-
Notifications
You must be signed in to change notification settings - Fork 50
Migrate from OpenCensus library #1881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
aa7bce5
to
6703484
Compare
c080533
to
61c5271
Compare
/test all |
89f4b05
to
8d2005f
Compare
d32a487
to
7dfe3c9
Compare
/test all |
7dfe3c9
to
c8772b9
Compare
9b6aeca
to
8544c2f
Compare
} | ||
|
||
func TestApply(t *testing.T) { | ||
_ = testmetrics.NewTestExporter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this line doing? Consider adding a comment explaining the purpose
// Add the k8s.container.name resource label so that the google cloud monitoring | ||
// and monarch metrics exporters will use the k8s_container resource type | ||
// RegisterOTelExporter creates the OTLP metrics exporter. | ||
func RegisterOTelExporter(containerName string) (*otlpmetricgrpc.Exporter, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should accept a context so it can pass it along
Tags: []tag.Tag{ | ||
{Key: metrics.KeyInternalErrorSource, Value: "parser"}, | ||
}, | ||
Name: "declared_resources", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a change in behavior?
) | ||
|
||
// RecordKustomizeFieldCountData records all data relevant to the kustomization's field counts | ||
func RecordKustomizeFieldCountData(ctx context.Context, fieldCountData *KustomizeFieldMetrics) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you moved all this code to the other file? It would probably simplify the diff/review if this was kept the same or refactored separately
Same applies in the other places where a similar refactor was done
if commit == "" { | ||
// TODO: Remove default value when otel-collector supports empty tag values correctly. | ||
commit = CommitNone | ||
commit = "NONE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this switch off the const?
view.RegisterExporter(&e) | ||
return &e | ||
// CollectMetrics collects all OpenTelemetry metrics and stores them in a simple format | ||
func (e *TestExporter) CollectMetrics() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this function accept a context?
} | ||
|
||
// Global test reader to capture metrics | ||
var globalTestReader sdkmetric.Reader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the global reader necessary? The previous scaffolding seems to have been able to do this without a global
Change is meant to be transparent to user. Using OTel SDK for metric composing in: kmetrics, core metrics, resource group metrics; Using otlp receiver in otel-agent and otel-collector; Configured deployment for new ports and component; Refactor metric composing and recording; Metric prefix remain the same to minimize breaking change. Tests updated.
8544c2f
to
ac1af18
Compare
/gemini summary |
Pull Request SummaryThis pull request migrates the project's metrics implementation from OpenCensus to OpenTelemetry SDK. The migration is comprehensive, ensuring that existing metric names, types, descriptions, recording patterns, pipeline architecture, data flow, sidecar configurations, and export destinations (Prometheus, Cloud Monitoring, Cloud Monarch) are preserved. Key changes include:
This migration ensures the project aligns with the latest observability standards and best practices provided by the OpenTelemetry ecosystem. Code ReviewOverall, this is a well-executed and comprehensive migration from OpenCensus to OpenTelemetry. The changes are consistent across the codebase, addressing all necessary aspects from dependency management to metric instrumentation, recording, exporting, and testing. The adherence to OpenTelemetry's API and best practices is evident throughout the modified files. I recommend merging this pull request. |
this is closed, but is there a follow up? Are we giving up on this project? |
The PR was opened on a wrong branch. Will reopen new one from fork and address comments from there. |
The migration replaces OpenCensus libraries with OpenTelemetry SDK while preserving:
Key Changes
1. Library Dependencies
Before (OpenCensus):
After (OpenTelemetry):
2. Metric Instrument Types
stats.Int64
metric.Int64Counter
stats.Int64
metric.Int64Gauge
stats.Float64
metric.Float64Histogram
3. Recording Patterns
Before (OpenCensus):
After (OpenTelemetry):
4. Tag/Attribute System
Before (OpenCensus):
After (OpenTelemetry):
5. Exporter Configuration
Before (OpenCensus):
After (OpenTelemetry):